Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @yadaven, @shay-berman, and @olgashtivelman)

controller/controller.go, line 530 at r2 (raw file):

	if (volumeBackend == resources.SpectrumScale) {
		volumeMountPoint, ok = volumeConfig["mountpoint"].(string)

note - you still don't sure if mountpoint is needed in the DB.
So i am not sure you can complete this until you know if you will have mountpoint in the DB for scale backend.

controller/controller_test.go, line 505 at r2 (raw file):

		It("should fail to Mount when there are errors from checking other slinks (idempotent) (doMount)", func() {
			err :=  fmt.Errorf("an Error has occured")
			fakeExec.GetGlobFilesReturns(nil, err)

I am expecting to see more UT to cover the new things you added to controller.go

because it looks like you have lack of UT here.

controller/errors.go, line 137 at r2 (raw file):

}

const MissingMountPointVolumeErrorStr = "MountPoint Missing in Volume Config."

since its scale error, then please add Scale as prefix to the const\type of this error.
So at least it will be clear that its Scale thing

First, please update the PR description with more detail about this PR content. So everyone will be able to understand what this PR offers.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @olgashtivelman)

controller/controller.go, line 530 at r2 (raw file):

ok good, so just clarify - you removed the mountpoint from the DB and instead everytime getVolumeConfig called it automatically get the mountpoint from the Scale system? (BTW i didn't see any PR that propose this fix, so please clarify it)

in addition, please make sure that you don't need to make it as 2 commands - volumemountpoint, ok = volumeConfig["mountpoint"]..... and then to make it a string. just make sure its ok. and add UT that check case that mountpoint key not exist.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @yadaven and @olgashtivelman)

controller/controller.go, line 714 at r5 (raw file):

		lnPath = k8sPVDirectory
	} else {
		lnPath = k8sPVDirectory

So why do you need the if\else here?
just drop it and lnPath=k8sPVDirctory

though? do i miss something here?

controller/controller.go, line 715 at r5 (raw file):

	} else {
		lnPath = k8sPVDirectory
		// Keeping this separate condition for scale-nfs support in future.

please put TODO prefix to this comment. but anyway you don't need if\else. in the future if you need it add it. but not now

controller/controller.go, line 963 at r5 (raw file):

        getVolumeRequest := resources.GetVolumeRequest{Name: volName}
        volume, err := c.Client.GetVolume(getVolumeRequest)

are you sure that we don't call to GetVolume already in the flow? if so it means we do so twice. please doublecheck to reduce the calls to ubiqutiy server.

maybe you can avoid hostattach in higher level and not here.

controller/controller.go, line 996 at r5 (raw file):

	// Only in k8s 1.5 this os.Hostname will happened,
	// because in k8s 1.5 the flex CLI doesn't get the host to attach with. TODO consider to refactor to remove support for 1.5
	// Spectrum Scale uses this method for 2.0 release.

why do you need this comment? what does it mean? is it being used for scale? if so how. try to be more specific in your comment here.

controller/controller_test.go, line 227 at r5 (raw file):

		It("should fail to prepareUbiquityMountRequest if GetVolumeConfig does not contain valid backend", func() {

please make sure you cover byunit testing all the new if\else you added

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @yadaven, @shay-berman, and @olgashtivelman)

controller/controller.go, line 714 at r5 (raw file):

I had kept that way because I wanted to keep condition to for scbe backend as is. And in future when we add nfs functionality only scale condition need to be added. But I agree with you comment. Also code doesnt look good this way. Hence as you suggested I will remove the redundancy and unnecessory check. In future whenever we add nfs support we will add required condition's again.

controller/controller.go, line 715 at r5 (raw file):

Agree

controller/controller.go, line 963 at r5 (raw file):

Yes, This is the only call for getVolume in Detach flow.

controller/controller.go, line 996 at r5 (raw file):

Agree. Added more details

•   // Spectrum Scale uses this method to get the hostname of current node for detach and isHostAttached functionality as Spectrum Scale volume is not attached to any specific node.
  

•   // TODO: Remove this dependancy while optimizing the Spectrum Scale detach functionality.
  

Reviewed 2 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @shay-berman)

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @shay-berman and @yadaven)

controller/controller.go, line 963 at r5 (raw file):

The current code fix both IsAttached and Detach functionality.
Other option is
in Detach function itself before calling doDetach() get the backend by calling GetVolume() and for spectrumscale backend return success.

Above will only fix the Detach for spectrumscale.
To Fix the IsAttached functionality - In IsAttached() before calling doIsAttached(), get the backend by calling GetVolume() and for spectrumscale backend return success.

Please suggest which option to go for?

Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @olgashtivelman, @yadaven, and @shay-berman)

controller/controller.go, line 963 at r5 (raw file):

twice
as mentioned in the call
please rebase from dev - because u r working on old code. so first rebase, then make sure your solution is ok.

controller/controller.go, line 712 at r6 (raw file):

	lnPath = k8sPVDirectory
	// TODO: Keeping this function for scale-nfs support in future.
	return lnPath

so this function do nothing.
if u want for future use to keep it its ok. but then please just do one line : return k8sPVDirectory

Reviewable status: 3 of 5 files reviewed, 5 unresolved discussions (waiting on @olgashtivelman, @shay-berman, and @yadaven)

controller/controller.go, line 264 at r16 (raw file):

Agree, Added notSupportedFlexVolumeResponse as well as used successFlexVolumeResponse wherever required.

Reviewable status: 3 of 5 files reviewed, 5 unresolved discussions (waiting on @olgashtivelman and @shay-berman)

Hi Deepack, I just reviewed your PR and you need to fix some aspects. Please review my comments.

Reviewable status: 3 of 5 files reviewed, 19 unresolved discussions (waiting on @olgashtivelman and @yadaven)

glide.yaml, line 113 at r17 (raw file):

  - lib
- package: github.com/IBM/ubiquity
  version: 19891dbfbc526a5f56e87bcccb6342f638dd8080

just make sure its the latest ubiqutiy commit in dev.
I think its now 422904c4c777d359341f22687ab6e3c9b459c470

cmd/provisioner/main/main.go, line 46 at r17 (raw file):

		if we ever move to a newer code version this can be removed.
	*/  
	flag.CommandLine.Parse([]string{})

why this code is in this PR?
isn't it a code that already exist in dev? (please make sure you will not have collision)

controller/controller.go, line 712 at r6 (raw file):

second though, just delete this function. its do nothing. instead just use the k8sPVDirectory.
In next version if you want to support NFS then do this , but not now.

controller/controller.go, line 137 at r17 (raw file):

	if err != nil {
		errormsg := fmt.Sprintf("Failed to get Volume details [%s]", attachRequest.Name)

please Add comment

TODO, later we should consider to get the volume backend by using the attachRequest.opts instead of calling to ubiqutiy server.

controller/controller.go, line 143 at r17 (raw file):

	if (volume.Backend == resources.SpectrumScale) {
            	response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")

Please clarify the text :
"Flex Attach API is not supported for Spectrum-Scale backend, because the backend assume the volume already attached to the node"
Please make sure my english is correct.

controller/controller.go, line 208 at r17 (raw file):

	getVolumeRequest := resources.GetVolumeRequest{Name: volName}
	volume, err :=  c.Client.GetVolume(getVolumeRequest)

You forgot to handle idempotancy here as mentioned in the requirement: "add GetVolume if scale then not support (if PV not exist then just return error)"

So you need to check if getvolume return error because PV not exist in ubiqutiy-DB, and if so(PV not in ubiqutiy-db) then you should continue OK with idempotent warning (but if getvolume failed due to other error then you should fail with error). Exactly the same as -> https://github.com/IBM/ubiquity-k8s/blob/master/controller/controller.go#L457

Also please add here a comment with # TODO, later we should consider to get the volume backend by using the IsAttachRequest.opts instead of calling to ubiqutiy server.

controller/controller.go, line 218 at r17 (raw file):

	if (volume.Backend == resources.SpectrumScale) {
        	response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")

please fix the text, with the same spirit as mentioned above

controller/controller.go, line 257 at r17 (raw file):

		if err != nil {
            		response = c.successFlexVolumeResponse("Returning success since volume does not exist")

wait you have here the same bug as mentioned above.
you need to check if the getvolume error is specifically mentioned that PV not exist, and only then you should return success with warnning idempotancy message.
please fix, it should be the same as https://github.com/IBM/ubiquity-k8s/blob/master/controller/controller.go#L457

controller/controller.go, line 262 at r17 (raw file):

	    	if (volume.Backend == resources.SpectrumScale) {
            		response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")

please fix text as mentioned above.

controller/controller.go, line 263 at r17 (raw file):

	    	if (volume.Backend == resources.SpectrumScale) {
            		response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")
			return response

I think you have indentation issue. you forgot few spaces for the return. (try to use gofmt in your IDE)

controller/controller.go, line 449 at r17 (raw file):

		return fmt.Sprintf(resources.PathToMountUbiquityBlockDevices, volumeConfig["Wwn"].(string)), nil
	} else if volumeBackend == resources.SpectrumScale {
                return volumeConfig["mountpoint"].(string), nil

Add comment:
#TODO, scale should remove the mountpoint.

controller/controller.go, line 544 at r17 (raw file):

	if (volume.Backend == resources.SpectrumScale) {
		return c.successFlexVolumeResponse("")
	}

I prefer more explicit here. So it will be clear that its different for scale vs scbe and also check if its other backend (if so then fail with not supported backend.)
something like this:

if (volume.Backend == resources.SpectrumScale) {
            # Scale has no detach process after unmount
	return c.successFlexVolumeResponse("")
    } else if (volume.Backend == resources.SCBE) {
    // Do legacy detach (means trigger detach as part of the umount from the k8s node)
	    if err := c.doLegacyDetach(unmountRequest); err != nil {
  	return c.failureFlexVolumeResponse(err, "")
    } 

 
return c.successFlexVolumeResponse("")
} else {

print some error like backend not supported (I think there is already error like that you can leverage -> see PvBackendNotSupportedError

)
}

In addition, I would expect to see UT for this scale if\else

controller/controller.go, line 589 at r17 (raw file):

}

func (c *Controller) getMountpointForVolume(mountRequest k8sresources.FlexVolumeMountRequest, volumeConfig map[string]interface{}, volumeBackend string) (string, error) {

please make sure u have UT for this

controller/controller.go, line 977 at r17 (raw file):

		// we can go back to using regulat getHostAttached and remove getHostAttachUsingConfig.
		getVolumeConfigRequest := resources.GetVolumeConfigRequest{Name: detachRequest.Name, Context: detachRequest.Context}
		volumeConfig, err := c.Client.GetVolumeConfig(getVolumeConfigRequest)

what is this code?
how its related to PR detail? it looks like olga code that is already in DEV.
please check it out

controller/controller.go, line 989 at r17 (raw file):

		if host == "" {
			// this means that the host is not attached to anything so no reason to call detach
			c.logger.Warning(fmt.Sprintf("Idempotent issue encoutered - vol is not attached to any host. so no detach action is called"), logs.Args{{"vol wwn", volumeConfig["Wwn"]}})

also this one?

controller/controller.go, line 1022 at r17 (raw file):

}

func (c *Controller) getHostAttachUsingConfig(volumeConfig map[string]interface{}) (string, error){

is this related to scale changes?

controller/controller_test.go, line 1031 at r17 (raw file):

		})

        It("IsAttached should return success with Attached as false when GetVolume Fails", func() {

need to fix it

it should succeed only if getvolume return error that the PV not in ubiqutiy DB, else it should fail. as mentioned above. please fix UT according.

controller/controller_test.go, line 1068 at r17 (raw file):

		})

        It("Pass when Detach fails to get volume details", func() {

only if PV not exist in DB error, else it should fail.

Reviewable status: 3 of 5 files reviewed, 19 unresolved discussions (waiting on @olgashtivelman, @yadaven, and @shay-berman)

controller/controller.go, line 222 at r17 (raw file):

	}

	isAttached, err := c.doIsAttached(isAttachedRequest)

BTW
please check that the detach\attach or mount\unmount doesn't use this doIsAttached - because if so then maybe you should skip calling it if its scale backend.
please check and let me know if it could lead you to an issue.

thanks

controller/controller.go, line 1022 at r17 (raw file):

maybe its reviewable issue. it look like olga code and i can see it here as added into the PR. So i think its ok.

Reviewable status: 1 of 5 files reviewed, 17 unresolved discussions (waiting on @olgashtivelman, @yadaven, and @shay-berman)

controller/controller.go, line 712 at r6 (raw file):

Done

controller/controller.go, line 137 at r17 (raw file):

Done

controller/controller.go, line 143 at r17 (raw file):

done

controller/controller.go, line 218 at r17 (raw file):

Done

controller/controller.go, line 262 at r17 (raw file):

Done

controller/controller.go, line 263 at r17 (raw file):

Done

controller/controller.go, line 449 at r17 (raw file):

Done

Well done
I will trigger staging now. after staging is green then you will be able to merge to dev.

Reviewed 1 of 1 files at r9, 2 of 4 files at r18, 2 of 2 files at r19.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @olgashtivelman)